Skip to content
This repository has been archived by the owner on Mar 9, 2022. It is now read-only.

Move to go modules #1377

Closed
wants to merge 6 commits into from
Closed

Move to go modules #1377

wants to merge 6 commits into from

Conversation

chenrui333
Copy link
Contributor

@k8s-ci-robot
Copy link

Hi @chenrui333. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@AkihiroSuda
Copy link
Member

Thanks, but I think we should begin with updating vendored pkgs with vndr

go.mod Outdated
github.com/containerd/typeurl v0.0.0-20190228175220-2a93cfde8c20
github.com/containernetworking/plugins v0.7.6
github.com/davecgh/go-spew v1.1.1
github.com/docker/distribution v2.7.1+incompatible
Copy link
Member

@AkihiroSuda AkihiroSuda Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you need to vendor master

# github.com/containerd/cri/pkg/util
pkg/util/image.go:32:9: undefined: reference.ParseDockerRef
Makefile:87: recipe for target '_output/containerd' failed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requested shipping v2.8.0 here: distribution/distribution#3085

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @dmcgowan

go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@chenrui333
Copy link
Contributor Author

will circle back tonight.

@chenrui333
Copy link
Contributor Author

Rebased the change to the latest master branch and build with v1.13.9

@chenrui333
Copy link
Contributor Author

As I have upgraded docker/distribution to go1.14, also going to bump this PR to v1.14.

@ibuildthecloud
Copy link
Contributor

ibuildthecloud commented Apr 24, 2020

A bit more info here containerd/containerd#4209 (comment) but in short containerd/cri can not update to modules until the dependency on k8s.io/kubernetes is removed. If you depend on k8s.io/kubernetes you are going to make everybodies life difficult if they want to use the containerd client.

@mikebrow
Copy link
Member

pls rebase.. we are off travis now.. and on github actions.. https://github.com/containerd/cri/blob/master/.github/workflows/ci.yml

@dims
Copy link
Member

dims commented Apr 25, 2020

fyi what it would take to remove k8s.io/kubernetes dependency - #1463

@chenrui333
Copy link
Contributor Author

pls rebase.. we are off travis now.. and on github actions.. https://github.com/containerd/cri/blob/master/.github/workflows/ci.yml

Nice!!

@AkihiroSuda
Copy link
Member

Needs rebase.

Anyone interested in carrying this? As we have removed dependency on k/k (thanks @dims), this one is probably easy now.

@dims
Copy link
Member

dims commented Jun 30, 2020

@AkihiroSuda we should probably concentrate on collapsing this repo with containerd/containerd and do it there?

@AkihiroSuda
Copy link
Member

Repo unification might not happen until containerd v1.5 (#1479 (comment)), but migrating to go.mod (of CRI plugin, at least) seems safe to have in v1.4.

@dims
Copy link
Member

dims commented Jun 30, 2020

@AkihiroSuda i read that comment as - we can do the collapse as soon as we cut a release branch for 1.4 :) which is hopefully soon.

we can always experiment if someone has the time here/now!

@chenrui333
Copy link
Contributor Author

Needs rebase.

Anyone interested in carrying this? As we have removed dependency on k/k (thanks @dims), this one is probably easy now.

I am still here, doing this now.

go.mod Outdated Show resolved Hide resolved
.appveyor.yml Outdated Show resolved Hide resolved
.appveyor.yml Outdated Show resolved Hide resolved
@chenrui333
Copy link
Contributor Author

Based on the feedback I got, I am reverting back to go 1.13.x line.

@chenrui333
Copy link
Contributor Author

Currently failed with

# github.com/containerd/cri/pkg/server
##[error]Makefile:84: recipe for target '_output/containerd' failed
##[error]pkg/server/service_unix.go:37:5: undefined: "github.com/containerd/containerd/sys".RunningInUserNS
make: *** [_output/containerd] Error 2

@thaJeztah
Copy link
Member

that's weird; the go mod specifies containerd v1.4.0-beta.0 (we should probably update to beta.1), but that version should have the RunningInUserns; https://github.com/containerd/containerd/blob/v1.4.0-beta.0/sys/userns_linux.go#L33

@chenrui333
Copy link
Contributor Author

let me try a new rebase again.

chenrui333 and others added 2 commits July 1, 2020 00:47
include backport fix

remove k8s.io/kubernetes (per #1463)

remove `github.com/docker/distribution` dependency

containerd/containerd#4251 and #1484

Signed-off-by: Rui Chen <[email protected]>
@chenrui333
Copy link
Contributor Author

Can I get a restart for this build, CI / Build, Unit, Integration, and CRI (linux amd64) (pull_request)?

After this operation, 521 kB of additional disk space will be used.
Ign:1 http://azure.archive.ubuntu.com/ubuntu bionic-updates/main amd64 libseccomp-dev amd64 2.4.1-0ubuntu0.18.04.2
Err:1 http://security.ubuntu.com/ubuntu bionic-updates/main amd64 libseccomp-dev amd64 2.4.1-0ubuntu0.18.04.2
  404  Not Found [IP: 52.177.174.250 80]
E: Failed to fetch http://security.ubuntu.com/ubuntu/pool/main/libs/libseccomp/libseccomp-dev_2.4.1-0ubuntu0.18.04.2_amd64.deb  404  Not Found [IP: 52.177.174.250 80]
E: Unable to fetch some archives, maybe run apt-get update or try with --fix-missing?
##[error]Process completed with exit code 100.

@thaJeztah
Copy link
Member

I don't think I have access / a way to restart (might want to push again to trigger).

That said, I do see some failures on AppVeyor that look relevant;

# k8s.io/klog/v2
C:\gopath\pkg\mod\k8s.io\klog\[email protected]\klog.go:690:45: undefined: logr.InfoLogger
C:\gopath\pkg\mod\k8s.io\klog\[email protected]\klog.go:702:43: undefined: logr.InfoLogger
C:\gopath\pkg\mod\k8s.io\klog\[email protected]\klog.go:706:48: undefined: logr.InfoLogger
C:\gopath\pkg\mod\k8s.io\klog\[email protected]\klog.go:721:44: undefined: logr.InfoLogger
C:\gopath\pkg\mod\k8s.io\klog\[email protected]\klog.go:739:55: undefined: logr.InfoLogger
C:\gopath\pkg\mod\k8s.io\klog\[email protected]\klog.go:764:32: undefined: logr.InfoLogger
C:\gopath\pkg\mod\k8s.io\klog\[email protected]\klog.go:881:43: undefined: logr.InfoLogger
C:\gopath\pkg\mod\k8s.io\klog\[email protected]\klog.go:1234:10: undefined: logr.InfoLogger
Makefile:84: recipe for target '_output/containerd.exe' failed
mingw32-make: *** [_output/containerd.exe] Error 2

Do we need to set GOFLAGS="-mod=vendor" to force the vendor directory being used?

@thaJeztah
Copy link
Member

Hm.. actually, looks like it's not there in the vendored version as well. I see InfoLogger was removed in go-logr/logr@c53dffe, which was in v0.2.0 https://github.com/go-logr/logr/releases/tag/v0.2.0

Looks like klog has v0.1.0 in its go.mod, but our vendor.conf updated it to v0.2.0 in 41f184f. Looking at kubernetes v1.19.0-beta.2, it actually depends on v0.1.0 https://github.com/kubernetes/kubernetes/blob/v1.19.0-beta.2/go.mod#L255 https://github.com/kubernetes/kubernetes/blob/v1.19.0-beta.2/go.mod#L255, so that looks to be the wrong version that was vendored.

Not sure if kubernetes already switched to klog v2.2.0, because that version looks to have updated to the newer (v0.2.0) version; https://github.com/kubernetes/klog/releases/tag/v2.2.0

@thaJeztah
Copy link
Member

kubernetes master looks to be still on klog v2.1.0 (https://github.com/kubernetes/kubernetes/blob/a6a3bf2eb4a0b7e9f86f991ffc24b72beafce2f4/go.mod#L128) and logr v0.1.0; https://github.com/kubernetes/kubernetes/blob/a6a3bf2eb4a0b7e9f86f991ffc24b72beafce2f4/go.mod#L254

@dims what's the procedure to update that dependency?

@dims
Copy link
Member

dims commented Jul 1, 2020

@thaJeztah it's happening here kubernetes/kubernetes#92554

@thaJeztah
Copy link
Member

Ah, perfect! I think for now we could downgrade logr to v0.1.0 (if that helps making CI pass)

@mikebrow
Copy link
Member

mikebrow commented Jul 1, 2020

Do we need to set GOFLAGS="-mod=vendor" to force the vendor directory being used?

yes.. see comment ^ #1377 (comment)

@mikebrow
Copy link
Member

mikebrow commented Jul 1, 2020

re-running jobs..

@mikebrow
Copy link
Member

mikebrow commented Jul 1, 2020

on the validate vendor fail.. not sure if ../project validate vendor should work or not. Might have to switch to a different validator to get passed that one.

Signed-off-by: Rui Chen <[email protected]>
Signed-off-by: Rui Chen <[email protected]>
@thaJeztah
Copy link
Member

Looks like we're getting closer; I see vendor validation is failing:

 ../project/script/validate/vendor
2020/07/01 18:54:44 Collecting initial packages
2020/07/01 18:54:44 Download dependencies
2020/07/01 18:54:47 Starting whole vndr cycle because no package specified
...

And that is because this PR adds a go.mod, but doesn't remove the vendor.conf. The validation script does this check: https://github.com/containerd/project/blob/6cc8c883e567db6fff1adb651e1ccee4c80adb6f/script/validate/vendor#L20-L23

if [ -f vendor.conf ]; then
  rm -rf vendor/
  vndr |& grep -v -i clone
else
  go mod tidy
...
...

So if it finds a vendor.conf if assumes that that's the vendoring tool to use, so if you can remove the vendor.conf (preferably in the same commit as the one that's adding the go.mod), then that check should likely pass.

@mikebrow
Copy link
Member

mikebrow commented Jul 1, 2020

Looks like we're getting closer; I see vendor validation is failing:

 ../project/script/validate/vendor
2020/07/01 18:54:44 Collecting initial packages
2020/07/01 18:54:44 Download dependencies
2020/07/01 18:54:47 Starting whole vndr cycle because no package specified
...

And that is because this PR adds a go.mod, but doesn't remove the vendor.conf. The validation script does this check: https://github.com/containerd/project/blob/6cc8c883e567db6fff1adb651e1ccee4c80adb6f/script/validate/vendor#L20-L23

if [ -f vendor.conf ]; then
  rm -rf vendor/
  vndr |& grep -v -i clone
else
  go mod tidy
...
...

So if it finds a vendor.conf if assumes that that's the vendoring tool to use, so if you can remove the vendor.conf (preferably in the same commit as the one that's adding the go.mod), then that check should likely pass.

and if he does https://github.com/containerd/cri/blob/master/hack/utils.sh#L26 and https://github.com/containerd/cri/blob/master/hack/utils.sh#L77 will likely need to be tweaked ... Note if I remember right k8s uses our vendor.conf @dims heads up.

@thaJeztah
Copy link
Member

oh, good catch; perhaps (just for testing) rename vendor.conf to see if everything else would work, and then work out how to change that utils.sh

@chenrui333
Copy link
Contributor Author

Sounds good, renaming vendor.conf now

@thaJeztah
Copy link
Member

oh! I think my comment was a bit ambiguous; the utils.sh script still has to be modified (to account for the rename, as it now fails), but mostly meant: "let's look at modifying it to work with go.mod later".

##[error]awk: cmd. line:2: fatal: cannot open file `/home/runner/work/cri/cri/src/github.com/containerd/cri/hack/../vendor.conf' for reading (No such file or directory)

This needs a rebase now (some dependencies were updated in vendor.conf, so need to be updated in go.mod, and dependencies need to be re-vendored)

@AkihiroSuda
Copy link
Member

needs rebase

@chenrui333
Copy link
Contributor Author

missing this earlier, I will do that after my evening walk :)

@chenrui333
Copy link
Contributor Author

will redo the commits tomorrow. (rebase is more than a headache)

@chenrui333
Copy link
Contributor Author

sorry for dropping the ball a little bit, I will do something tonight about this PR.

@dmcgowan
Copy link
Member

dmcgowan commented Oct 7, 2020

The master branch on this repository has been merged into containerd/containerd. This can be tried directly in the main containerd repo now.

@dmcgowan dmcgowan closed this Oct 7, 2020
@chenrui333
Copy link
Contributor Author

🆒 Thanks!!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants